Skip to content

Modify AES-CTR to not reinit after being keyed#213

Merged
douzzer merged 3 commits intowolfSSL:masterfrom
ColtonWilley:aes_ctr_dont_reinit_after_keying
Jun 10, 2024
Merged

Modify AES-CTR to not reinit after being keyed#213
douzzer merged 3 commits intowolfSSL:masterfrom
ColtonWilley:aes_ctr_dont_reinit_after_keying

Conversation

@ColtonWilley
Copy link
Copy Markdown
Contributor

No description provided.

@ColtonWilley
Copy link
Copy Markdown
Contributor Author

ColtonWilley commented Jun 5, 2024

Retest this please

@SparkiDev
Copy link
Copy Markdown
Contributor

Is there an explicit test for this changes?

Comment thread test/test_cipher.c Outdated
@ColtonWilley
Copy link
Copy Markdown
Contributor Author

Is there an explicit test for this changes?

This re-init flow already exists in test_aes_ctr_leftover_data_regression(). It used to work before commit wolfSSL/wolfssl@6b8280f, but that commit changes wc_AesInit() to set rounds to zero, thus causing a new but valid error from wc_AesEncrypt() because rounds is zero.

@kaleb-himes
Copy link
Copy Markdown
Contributor

kaleb-himes commented Jun 6, 2024

Is there an explicit test for this changes?

This re-init flow already exists in test_aes_ctr_leftover_data_regression(). It used to work before commit wolfSSL/wolfssl@6b8280f, but that commit changes wc_AesInit() to set rounds to zero, thus causing a new but valid error from wc_AesEncrypt() because rounds is zero.

I may have misunderstood but I think @SparkiDev was asking if an explicit test will be added to cover the case.

The issue identified came from the call sequence: init-with-key then init-iv where the init-iv wiped out the rounds set by init-with-key

Possibly adding a test in tests/api.c that checks the rounds after a double init call? (Pseudo code below)

int test_aes_ctr_multi_init()
{
     EXPECT_DECLS;
     int rounds_expected = #rounds expected to be there as a result of the first call init'ing with key;
     /* Test valid sequence */
     ExpectIntEQ(we_aes_ctr_init(with key set), WOLFSSL_SUCCESS);
     ExpectIntEQ(we_aes_ctr_init(with all the NULL params and only the IV passed in), WOLFSSL_SUCCESS);
     ExpectIntEQ(rounds in the ctx, rounds_expected);
     return EXPECT_RESULT();
}

@ColtonWilley
Copy link
Copy Markdown
Contributor Author

Possibly adding a test in tests/api.c that checks the rounds after a double init call?

If such tests were to be added, that should be a direct test of the wolfcrypt APIs, I dont think wolfengine tests are a good place for verifying AES context internals. We could add another test with just a double init, but it would ultimately be a copy of the existing test code. If the consensus is that we absolutely want new test code I can add some, but I think it would be a copy of existing code that doesnt actually add anything new in terms of testing.

@kaleb-himes
Copy link
Copy Markdown
Contributor

Fair enough.

@bandi13
Copy link
Copy Markdown
Contributor

bandi13 commented Jun 6, 2024

The failure in gcc, clang, scan-build, fsanitize and disable test(s) can be ignored. It was a test that accidentally ran on this PR. It has been disabled.

@ColtonWilley ColtonWilley requested a review from wolfSSL-Bot June 7, 2024 20:35
Copy link
Copy Markdown
Contributor

@douzzer douzzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. as discussed, more fixes will be needed for more leaks around wc_AesInit(), and possibly for more over-inits.

@douzzer douzzer merged commit 70670a5 into wolfSSL:master Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants